container provider: add External Logging Support SupportFeature #13319

enoodle · 2016-12-26T13:31:18Z

Adding common logging launch as a feature mixin.
Provider link will land on pre configured dashboard that is controlled in common_logging_path

resubmmiting #10648 without the UI part.
corresponding UI PR: ManageIQ/manageiq-ui-classic#36

miq-bot · 2017-01-03T21:31:43Z

<pr_mergeability_checker />This pull request is not mergeable. Please rebase and repush.

chessbyte · 2017-01-03T22:24:19Z

app/models/manageiq/providers/container_manager.rb

@@ -23,6 +24,12 @@ class ContainerManager < BaseManager

    virtual_column :port_show, :type => :string

+    supports :common_logging do
+      unless respond_to?(:common_logging_route_name)
+        unsupported_reason_add(:common_logging, _('This provider type doesn\'t support common_logging'))


Please change contraction doesn't to does not

chessbyte · 2017-01-03T22:25:04Z

app/models/container_node.rb

+
+  supports :common_logging do
+    unless ext_management_system.respond_to?(:common_logging_route_name)
+      unsupported_reason_add(:common_logging, _('This provider type doesn\'t support common_logging'))


Please change contraction doesn't to does not

simon3z · 2017-01-19T22:47:26Z

app/models/manageiq/providers/openshift/container_manager.rb

+  end
+
+  def common_logging_path
+    '/#/dashboard/Operations-Logs-Overivew'


@enoodle is this dashboard always available? Because IIUC it's not shipped with Kibana.

cc @portante

@simon3z I agree, I changed this to '/' (default entry screen of kibana). We should find a way to inject those dashboards and then have another PR to change this accordingly.

enoodle · 2017-01-24T07:56:18Z

ping @dkorn, @moolitayer , @zeari Please review

zeari · 2017-01-24T13:33:32Z

app/models/manageiq/providers/openshift/container_manager.rb

@@ -20,6 +20,18 @@ def self.event_monitor_class
    ManageIQ::Providers::Openshift::ContainerManager::EventCatcher
  end

+  def common_logging_route_name
+    Settings.container_logging.common_logging_route
+  end


Think about going with Settings.container_logging.try(:common_logging_route) and handling a nil since I dont know if existing miq instances will re-seed the config/settings.yml file

👍 I will have to try for container_logging too.

handling the nil result will be in the UI PR (huh...)

simon3z · 2017-01-24T18:31:27Z

app/models/container_node.rb

+  end
+
+  def common_logging_path
+    hostname = name.split('.')[0]


@portante @enoodle are we sure about this logic? It seems odd to me.
@enoodle also please handle hostname when is nil.

I think we want a query that looks like: "hostname:" OR "hostname:<value.split('.')[0]>"

@enoodle @portante do we know officially what this field hostname should contain?
Isn't there another field to filter by where the FQDN is used?

I know chances are low... but aren't you worried of collisions of hostnames if you're not using FQDN? (And also with FQDN there could be collisions, but there it's more a deployment issue).

First, logs almost never have perfect metadata, and so while we strive to have an FQDN in the hostname field, as is the case already in OpenShift 3.4, some parts of the system send an FQDN in, some just $(hostname -s). So, yes, collisions are there, but usually there is other metadata in the logs that help to distinguish the log entries.

I handled the case that name is nil and changed to a query of both the hostname and the fqdn.

simon3z · 2017-01-24T18:31:40Z

@miq-bot assign enoodle

simon3z · 2017-01-25T17:08:46Z

app/models/container_node.rb

+  end
+
+  def common_logging_path
+    hostname = (name || "").split('.').first


@enoodle for me this is still:

> name = nil > (name || "").split('.').first => nil

am I missing something?

@enoodle also you missed a comment from @portante about doing an OR.

@simon3z I should have explained myself better:
I changed the query below to filter for an OR between name and hostname. If name will be nil then hostname will be nil as well and their #{name} will return an empty string, which will cause this query to match all values and the user will be presented with the regular discovery page presenting all of the logs.
do you think I should add this as a comment?

Edit: No, I am sorry, The query won't catch all, it will be wrong, and an error message will be shown in Kibana about not being able to parse the query, but all the logs will be shown.

simon3z · 2017-01-26T16:08:06Z

@portante can you ack this PR if it looks good to you?

enoodle · 2017-01-30T11:38:50Z

@simon3z @portante ping

enoodle · 2017-01-31T12:23:15Z

@simon3z @portante I split the container node to another PR #13704

simon3z · 2017-01-31T21:33:20Z

LGTM 👍

But @Fryguy had a question on the common_logging_route setting on the other PR #13704 which is similar to this one.

Even though in theory it is correct that the setting should be per-provider, on the other hand OpenShift has no documentation on how to change the name of the route which at the moment is hard-coded in all the deployment scripts

Not sure if it's worth a per-provider setting ATM.

enoodle · 2017-02-02T20:42:42Z

ping, tests were failing due to a problem in brakeman (?) , I rebased.

enoodle · 2017-02-06T10:58:04Z

ping @Fryguy

simon3z · 2017-02-06T11:44:33Z

@enoodle can you check if this works with an "OpenShift Container Platform" provider (vs. just "OpenShift Origin")?

To me it seems that with "OpenShift Container Platform" is not working.

simon3z · 2017-02-06T11:44:54Z

@enoodle can you check if this works with an "OpenShift Container Platform" provider (vs. just "OpenShift Origin")?

To me it seems that with "OpenShift Container Platform" is not working.

@enoodle or maybe it's just a UI issue (to be continued on the other PR)

enoodle · 2017-02-06T11:57:56Z

@simon3z I will check, can you make sure you have both this PR and the UI counterpart?

simon3z · 2017-02-06T13:25:38Z

@simon3z I will check, can you make sure you have both this PR and the UI counterpart?

@enoodle I am looking at this on the setup that you helped @moolitayer with few week ago.

For "OpenShift Origin" providers I can see the link for the "OpenShift Container Platform" one I can't.

Please double check all the code.

enoodle · 2017-02-06T15:43:50Z

@simon3z fixed. I moved the kibana support definitions to openshift/container_manager_mixin.rb, as this is the common place between origin and the container platform provider.

simon3z · 2017-02-06T17:05:33Z

LGTM 👍

ping @Fryguy @chessbyte can you (review/)merge?

enoodle · 2017-02-08T11:13:20Z

ping @Fryguy @chessbyte

Fryguy · 2017-02-09T15:36:48Z

LGTM within just the context of this PR, so I think it's OK to merge. I have some high level questions regarding the overall design, though perhaps @simon3z we can discuss later? In particular,

Since a data warehouse provider was introduced, this feels like it overlaps there in some way.
I don't really understand the custom attribute versus settings discussion, though your hardcoding logic makes sense for now. My only concern was that a top level setting is a single setting for everything (including non-openshift providers) and you paint yourself into a corner the moment a customer has multiple kibana servers for whatever reason. Seems more prudent to design it with that in mind.
Not sure I like the supports feature dedicated to kibana. It feels like it should be supports_external_logging or something. I'm pretty sure I understand how Red Hat wants to use this feature, but I don't understand it from a pure platform perspective as it's designed so specifically.

Fryguy · 2017-02-09T15:37:30Z

@blomquisg Thoughts on the supports feature usage here?

enoodle · 2017-02-09T16:18:49Z

Since a data warehouse provider was introduced, this feels like it overlaps there in some way.

@Fryguy This is using the resources from the Openshift container manager to read the routes and direct the user to that link. The Datawarehouse manager will also be reading from Elasticsearch but here we want to give the user access to Kibana UI that is already deployed on the cluster. We need the feature because some container providers will not support route names yet (like kubernetes).

splitting common logging launch to a feature mixin

enoodle · 2017-02-10T18:04:08Z

@Fryguy @blomquisg I changed the name to External Logging Support, PTAL

simon3z · 2017-02-15T00:03:53Z

Any update on this @Fryguy @chessbyte @chessbyte ? Is there anything holding this back?

miq-bot · 2017-02-15T20:07:25Z

Checked commit enoodle@97ac328 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
3 files checked, 0 offenses detected
Everything looks good. 🏆

simon3z · 2017-02-22T08:44:41Z

Any update on this @Fryguy @chessbyte @chessbyte ? Is there anything holding this back?

@Fryguy @blomquisg @chessbyte can anyone explain why this is neglected for so long? It's 13 days that we're asking for a feedback.

blomquisg · 2017-02-22T19:43:14Z

@simon3z no ... sorry, the ping to me got lost in my emails and it wasn't assigned to me so I didn't see it ... I need a better way of sifting through the github notifications :/

This was referenced Dec 26, 2016

Adding External Logging links to container providers and nodes ManageIQ/manageiq-ui-classic#36

Closed

[WIP] container provider: adding a link to hosted kibana #10648

Closed

miq-bot added the unmergeable label Jan 3, 2017

chessbyte reviewed Jan 3, 2017

View reviewed changes

enoodle force-pushed the container_provider_common_logging_link_split branch from 7840754 to 1edab2f Compare January 4, 2017 14:40

miq-bot removed the unmergeable label Jan 4, 2017

chessbyte assigned simon3z Jan 5, 2017

chessbyte added enhancement providers/containers labels Jan 5, 2017

enoodle changed the title ~~container provider: adding a link to hosted kibana~~ container provider: Common Logging feature Jan 11, 2017

simon3z reviewed Jan 19, 2017

View reviewed changes

enoodle force-pushed the container_provider_common_logging_link_split branch from 1edab2f to f639edf Compare January 20, 2017 18:25

zeari reviewed Jan 24, 2017

View reviewed changes

enoodle force-pushed the container_provider_common_logging_link_split branch from f639edf to 201d9fd Compare January 24, 2017 17:00

simon3z reviewed Jan 24, 2017

View reviewed changes

miq-bot assigned enoodle and unassigned simon3z Jan 24, 2017

enoodle force-pushed the container_provider_common_logging_link_split branch from 201d9fd to e99e3bb Compare January 25, 2017 16:04

simon3z reviewed Jan 25, 2017

View reviewed changes

enoodle force-pushed the container_provider_common_logging_link_split branch from e99e3bb to 6feafda Compare January 27, 2017 12:07

enoodle force-pushed the container_provider_common_logging_link_split branch from 6feafda to 926e94d Compare January 31, 2017 12:17

enoodle mentioned this pull request Jan 31, 2017

External Logging link for container nodes #13704

Merged

miq-bot assigned enoodle and Fryguy and unassigned Fryguy and enoodle Feb 6, 2017

enoodle force-pushed the container_provider_common_logging_link_split branch 2 times, most recently from 595fb80 to 4c6b56a Compare February 6, 2017 15:34

enoodle force-pushed the container_provider_common_logging_link_split branch from 4c6b56a to a5a01e1 Compare February 6, 2017 15:46

enoodle changed the title ~~container provider: Common Logging feature~~ container provider: add Kibana Support SupportFeature Feb 8, 2017

container provider: adding a link to hosted kibana

97ac328

splitting common logging launch to a feature mixin

enoodle force-pushed the container_provider_common_logging_link_split branch from a5a01e1 to 97ac328 Compare February 10, 2017 18:00

enoodle changed the title ~~container provider: add Kibana Support SupportFeature~~ container provider: add External Logging Support SupportFeature Feb 10, 2017

blomquisg merged commit 81bd26a into ManageIQ:master Feb 22, 2017

blomquisg added this to the Sprint 55 Ending Feb 27, 2017 milestone Feb 22, 2017

container provider: add External Logging Support SupportFeature #13319

container provider: add External Logging Support SupportFeature #13319

Conversation

enoodle commented Dec 26, 2016 • edited by miq-bot Loading

miq-bot commented Jan 3, 2017

chessbyte Jan 3, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

enoodle commented Jan 24, 2017

zeari Jan 24, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simon3z commented Jan 24, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

enoodle Jan 25, 2017 • edited Loading

Choose a reason for hiding this comment

simon3z commented Jan 26, 2017

enoodle commented Jan 30, 2017

enoodle commented Jan 31, 2017

simon3z commented Jan 31, 2017

enoodle commented Feb 2, 2017

enoodle commented Feb 6, 2017

simon3z commented Feb 6, 2017

simon3z commented Feb 6, 2017

enoodle commented Feb 6, 2017

simon3z commented Feb 6, 2017

enoodle commented Feb 6, 2017

simon3z commented Feb 6, 2017

enoodle commented Feb 8, 2017

Fryguy commented Feb 9, 2017

Fryguy commented Feb 9, 2017

enoodle commented Feb 9, 2017

enoodle commented Feb 10, 2017

simon3z commented Feb 15, 2017

miq-bot commented Feb 15, 2017

simon3z commented Feb 22, 2017

blomquisg commented Feb 22, 2017

enoodle commented Dec 26, 2016 •

edited by miq-bot

Loading

chessbyte Jan 3, 2017 •

edited

Loading

zeari Jan 24, 2017 •

edited

Loading

enoodle Jan 25, 2017 •

edited

Loading